Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: Cleanup remaining userlib namespace imports #1857

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

chore: Cleanup remaining userlib namespace imports #1857

wants to merge 5 commits into from

Conversation

aapoalas
Copy link
Contributor

@aapoalas aapoalas commented Aug 23, 2024 •
edited
Loading

This PR ends my work of walking through the hubris code base, removing userlib namespace imports.

This time I did a search-and-replace on all of these to clear them out. I currently see some build issue about notifications with app/lpc55xpresso/app-sprot.toml; how the notifications should get built is not quite clear to me.

Another interesting file is task/template/src/main.rs where we have this comment:

// NOTE: you will probably want to remove this when you write your actual code;
// we need to import userlib to get this to compile, but it throws a warning
// because we're not actually using it yet!
#[allow(unused_imports)]
use userlib::{};

I'm not sure if I should add some imports here or not.

aapoalas
aapoalas commented Aug 23, 2024
View reviewed changes
drv/stm32xx-i2c/src/lib.rs
@@ -923,7 +923,7 @@ impl I2cController<'_> {

#[rustfmt::skip]
i2c.oar1.modify(|_, w| { w
.oa1en().clear_bit() // own-address disable
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: Zed editor did these format fixes automatically, so they're kind of "slipping in" on the side.

@aapoalas aapoalas changed the title chore: Remove the rest of userlib namespace imports chore: Cleanup remaining userlib namespace imports Aug 24, 2024
@aapoalas
Copy link
Contributor Author

aapoalas commented Oct 2, 2024 •
edited
Loading

@cbiffle My apologies for the ping: Would you be amenable to taking a look at this? All CI tests have been fully green, only app/lpc55xpresso/app-sprot.toml has some sort of local build failure about missing notifications that seemed like it might be a configuration failure in that board's part. It is not being tested by CI so I am not sure if that is even relevant.

@aapoalas aapoalas force-pushed the chore/userlib-namespace-imports branch from 633adda to c453676 Compare October 11, 2024 02:07
@aapoalas
Copy link
Contributor Author

The remaining lpc55xpresso app-sprot.toml dist xtask passes now as well. I had to bump some stack sizes and copy SP_TO_ROT_JTAG_DETECT_L and ROT_TO_SP_RESET_L pin definitions from oxide-rot-1's app.toml. I have no idea if these are correct, but it seems fairly likely that if the xpresso build is still being used then it is somehow accidentally pulling in the pin definitions from the rot-1 configuration, in which case they should still be correct.

Though oops, I only now actually checked whether app-sprot.toml works on the master branch and it doesn't, so I guess that was wasted work.

@aapoalas aapoalas mentioned this pull request Oct 11, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@aapoalas